Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't ship tests to npm #781

Closed
wants to merge 1 commit into from
Closed

Conversation

frantic
Copy link

@frantic frantic commented May 2, 2017

Currently, when I npm install protobufjs I get node_modules/protobufjs/tests with about 1.6Mb worth of files.

@dcodeIO
Copy link
Member

dcodeIO commented May 2, 2017

The last time I checked there were more people considering it good practice to include tests than the opposite. Not sure if this has changed in the meantime.

@frantic
Copy link
Author

frantic commented May 2, 2017

What's the benefit? Where can I read more?

I found this, but I think the point the selected answer is making about tests is pretty weak. Also, your package does have devDependencies (that are not installed when you npm install protobufjs), and the tests probably can't even be run without them.

@dcodeIO
Copy link
Member

dcodeIO commented May 2, 2017

The only actual benefit I see is if there is a dependent who is, for some reason, running protobuf.js tests from within their tests, for example because they override some of its functionality manually and want to be sure that it didn't break the original test case.

Regarding devDependencies: Well, that is true but you can always run npm install in the package's directory to make it work. The alternative here would be to make the tests its own module, but that sounds like some not absolutely necessary extra-work.

As a compromise I can try to reduce the overall size of the shipped test cases by removing generated files (these are the large bits), and let these generate once when installing dev dependencies or something like that.

@frantic
Copy link
Author

frantic commented May 3, 2017

Thanks for the response! I noticed tests because Flow complained about an invalid json inside protobufjs package, so I had to blacklist it in the [ignore] section of my .flowconfig.

a dependent who is, for some reason, running protobuf.js tests from within their tests

I think it's a very rare use case. If somebody needs to modify functionality they can just fork the package on Github?

protobufjs package is very popular, 15K downloads/day. I just wanted to flag this for you and see if it was intentional, because I've seen other packages unintentionally shipping large files (e.g. microsoft/react-native-code-push#652).

@frantic
Copy link
Author

frantic commented Jun 11, 2017

Hey @dcodeIO, I was wondering if you want to merge or close this? I presented my arguments which I still believe make sense.

If you are worried about breaking existing clients that depend on the tests that ship with npm package, maybe consider merging this PR into future major version?

@dcodeIO
Copy link
Member

dcodeIO commented Jun 11, 2017

When we exclude tests from npm, we can technically go even further and exclude everything else not necessary for a working npm package as well. Maybe through files in package.json. Hmm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants